Skip to content

PHPORM-174 Add doc for cache and locks #2909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Apr 26, 2024
Merged

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Apr 24, 2024

Copy link
Contributor

@jordan-smith721 jordan-smith721 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some style guide and convention fixes, plus a few rst errors. Happy to discuss any of this further if needed!

docs/cache.txt Outdated


.. code-block:: php
:emphasize-lines: 9,14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may have meant to highlight 9 and 13? The function names?

Suggested change
:emphasize-lines: 9,14
:emphasize-lines: 9,13

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 14 is correct, it's the line with return $this->cache->increment('counter');

Copy link
Member Author

@GromNaN GromNaN Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Both lines need to be udpated.

Suggested change
:emphasize-lines: 9,14
:emphasize-lines: 10,15

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be lines 10 and 15 to highlight the CacheManager line and the return this-> ... line

docs/cache.txt Outdated


.. code-block:: php
:emphasize-lines: 9,14
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 14 is correct, it's the line with return $this->cache->increment('counter');

GromNaN and others added 2 commits April 25, 2024 09:08
Co-authored-by: Jordan Smith <[email protected]>
@GromNaN GromNaN marked this pull request as ready for review April 25, 2024 08:59
@GromNaN GromNaN requested a review from a team as a code owner April 25, 2024 08:59
@GromNaN GromNaN requested a review from jmikola April 25, 2024 08:59
- **Required**. The database connection used to store cache items. It must be a ``mongodb`` connection.

* - ``collection``
- Default ``cache``. Name of the MongoDB collection to store cache items.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't pick up on this when reviewing the original PRs, so forgive me for asking this late: where does the database name come from? Are you relying on whatever is configured through the corresponding connection option (for both this and lock_collection)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The database name is set in the database connection configuration. Do you think we should add an option to change the database name for cache and lock?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also consider allowing a namespace for the collection and lock_collection options, and parsing out a database name by exploding on a dot character (no issue since it's prohibited in each part per Naming Restrictions.

Otherwise, I'm fine keeping this as-is and revisiting if/when someone asks for further customization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking this feature: PHPORM-176

'stores' => [
'mongodb' => [
'driver' => 'mongodb',
'connection' => 'mongodb',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should collection and various lock options be specified here?

Also, is there any significance to the line break between default and stores in the configuration array? I believe you're already emphasizing the default line, so I don't think a line break is necessary for readability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I rely on the default values. Do you think I should make a note of that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling out your reliance on default values makes sense to me.

I was mostly concerned with collection. The MongoLock constructor requires a value for its Collection parameter, and I didn't see where the default gets supplied.

Copy link
Member Author

@GromNaN GromNaN Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"connection" is required, "collection" has a default value which is cache. These 2 words are so similar that they can easily be confused.

The default is applied in the service provider.

$config['collection'] ?? 'cache',

Copy link
Contributor

@jordan-smith721 jordan-smith721 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more super minor things from a read through on the staging link!

Copy link
Contributor

@jordan-smith721 jordan-smith721 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after one more fix!

GromNaN and others added 2 commits April 26, 2024 17:10
@GromNaN GromNaN merged commit 64d6dc0 into mongodb:4.3 Apr 26, 2024
24 checks passed
@GromNaN GromNaN deleted the PHPORM-174 branch April 26, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants